Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add icon for interestgroups in frontpageview #2594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PeterJFB
Copy link
Contributor

@PeterJFB PeterJFB commented Mar 22, 2022

The new frontpage (webkom/lego-webapp#2311) shows three randomly selected interestgroups I need them:)

Note: This was the only required change in the backend, which is why I'm adding it straight into production

Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, just a bit of nitpicking then this is good!

I also do agree that this should be a generic endpoint. I do understand that you don't want to introduce more logic, even though I fear that it won't happen if you don't do it now (that might just be a good thing if we don't ever have a need for it though 🤷)

I did think about this a bit as well and came up with a method to do this both generic and fix the "stable" randomness that we need.

I figured that we can generate a hash on the client, f.ex. using their username and the current day/hour/week (however long we want to keep the current values), and send this hash to the endpoint. We can then create a random list if ids, and store it in redis with this hash as a key. Then, we can just fetch the existing list of ids from redis the next time around, and fetch those objects

Not sure if this is needed, but if you do want to this more generic and also simplify the client code, this might be an option.

lego/apps/users/views/abakus_groups.py Outdated Show resolved Hide resolved
@LudvigHz
Copy link
Member

LudvigHz commented May 1, 2022

Also please squash the commits, as the first one does not contribute anything anymore.

@PeterJFB PeterJFB requested a review from LudvigHz May 3, 2022 17:41
Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@ivarnakken ivarnakken added new-feature Pull requests that introduce or issues that suggest a new feature approved Pull requests that have been approved do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged need-before-2050 Pull requests that have been in progress for far too long labels Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged need-before-2050 Pull requests that have been in progress for far too long new-feature Pull requests that introduce or issues that suggest a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants